Skip to content

ARROW-11778: [Rust] Cast from LargeUtf8 to Numerical and temporal types#9571

Closed
ritchie46 wants to merge 5 commits into
apache:masterfrom
ritchie46:large_utf8_to_numerical
Closed

ARROW-11778: [Rust] Cast from LargeUtf8 to Numerical and temporal types#9571
ritchie46 wants to merge 5 commits into
apache:masterfrom
ritchie46:large_utf8_to_numerical

Conversation

@ritchie46

Copy link
Copy Markdown
Contributor

Sorry that the PR's are not more clustered, but they occur to me in the wild.

This PR allows casting from LargeUtf8 to numerical and temporal types. It also modifies the already existing string to temporal casts such that it uses the new faster from_trusted_length iterator API.

@ritchie46 ritchie46 force-pushed the large_utf8_to_numerical branch from ad24ce3 to cda7de3 Compare February 25, 2021 08:34

@nevi-me nevi-me left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions

Copy link
Copy Markdown

@nevi-me

nevi-me commented Feb 25, 2021

Copy link
Copy Markdown
Contributor

@ritchie46 you have unused imports somewhere

@nevi-me

nevi-me commented Feb 26, 2021

Copy link
Copy Markdown
Contributor

I'll fix the clippy lints, and merge this after #9425

@ritchie46

Copy link
Copy Markdown
Contributor Author

@nevi-me thanks. Sorry about the lints, I totally forgot them today.

@nevi-me nevi-me force-pushed the large_utf8_to_numerical branch from 3ca7ead to 312c082 Compare February 26, 2021 20:04
@alamb

alamb commented Feb 26, 2021

Copy link
Copy Markdown
Contributor

The integration test failure seems like it is unrelated to the changes in this PR: https://issues.apache.org/jira/browse/ARROW-11717


#[test]
fn test_cast_string_to_timestamp() {
let a = StringArray::from(vec![

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend adding coverage here to convert from LargeUtf8 string arrays as well - I don't think they are covered by these tests

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nevi-me I just fixed this test. I already modified the test with that intention but I apparently tested stringarray twice instead of stringarray and largestringarray

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ritchie46 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants